Conversation
Signed-off-by: acpana <8968914+acpana@users.noreply.github.com>
acpana
added
pr/no-changelog
github-actions
bot
added
the
theme/config
agent/testagent.go
Outdated
|
|
||
| portsConfig := randomPortsSource(t, a.UseTLS) | ||
|
|
||
| //a.Overrides = ` |
There was a problem hiding this comment.
We could turn this on for all agents but I'd recommend turning it off by default and just enabling it for the tests that need it.
There was a problem hiding this comment.
I think not having this is the cause of all the existing CI failures. However if you uncomment this then your new tests regarding checking the enforcement behavior of not allowing the PeerName field to be set would then fail.
I generally agree that the best course of action would be to update tests that do the nasty things to opt into the behavior.
There was a problem hiding this comment.
I think as of now, all tests that need this config option on have it so I should remove this commented block
mkeeler
left a comment
mkeeler
left a comment
There was a problem hiding this comment.
I understand the need to for the new config to bypass the restriction in the case of tests. However, it makes me think we have gotten the interfaces and boundaries between components wrong if we need to do this.
Do you know how many/which tests get broken if we just unconditionally restrict this? I think there are a few different ways tests should inject peered services.
- Within the
agent/consulpackage those tests can insert directly into the state store or they can callraftApplyon the server directly. - Outside of
agent/consulunit tests should use mocks that don't require real data injection (or an actual server) - Outside of
agent/consulintegration tests should set up multiple agents and let the peering replication stream get the data injected instead of doing it directly.
With that approach, we shouldn't need any configurability of allowing/disallowing setting the peer name through the RPC endpoint.
I realize that we may have too many tests to make that feasible for this PR which is why I would be curious which/how many tests it breaks to just always enforce the restriction.
agent/catalog_endpoint_test.go
Outdated
| "github.com/hashicorp/consul/testrpc" | ||
| ) | ||
|
|
||
| func TestCatalogRegister_DenyPeeringRegistration(t *testing.T) { |
There was a problem hiding this comment.
As the functionality changed is within agent/consul/catalog_endpoint.go, the unit test should live in agent/consul/catalog_endpoint_test.go.
There was a problem hiding this comment.
So I wanted to test turning off the setting at the agent level but also add more cases at the consul level; what do you think of the current approach?
Signed-off-by: acpana <8968914+acpana@users.noreply.github.com>
Signed-off-by: acpana <8968914+acpana@users.noreply.github.com>
acpana
changed the title
mkeeler
left a comment
mkeeler
left a comment
There was a problem hiding this comment.
I took a quick peak at the CI failures and they look legit. Mostly due to not being able to register services with peer names.
Signed-off-by: acpana <8968914+acpana@users.noreply.github.com>
Signed-off-by: acpana <8968914+acpana@users.noreply.github.com>
Signed-off-by: acpana <8968914+acpana@users.noreply.github.com>
Signed-off-by: acpana <8968914+acpana@users.noreply.github.com>
|
We discussed internally and we DO NOT WANT to include this change in |
Signed-off-by: acpana <8968914+acpana@users.noreply.github.com>
Signed-off-by: acpana <8968914+acpana@users.noreply.github.com>
3 participants
Signed-off-by: acpana 8968914+acpana@users.noreply.github.com
description
We probably want to block the
Catalog.Registerendpoint from taking in requests with anyPeerName(on nodes, services, checks).proposal
The approach taken here is to add a config value that is non user configurable. But our test agent/servers can turn it on.